chore: simplify, modernize (Go 1.26), update deps#128
Conversation
- fix: /health returned 200 during shutdown (should be unavailableStatusCode, same as /ready) - refactor: atomic.Pointer[bool] → atomic.Bool; drop new(bool) stores and pointer derefs - fix: named-plugin paths used svc from the ok-check then re-looked up via map index — now use svc directly - modernize: for-index loops over plg slice → range-over-value (Go 1.22) - fix: jobs handler now threads r.Context() into JobsState instead of context.Background() - modernize: report slice pre-sized to registry len instead of fixed cap 2
|
Warning Review limit reached
More reviews will be available in 49 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors the status package to use ChangesAtomic Type Migration and Handler Refactoring
CI Workflow Security and Coverage Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the status service HTTP handlers and plugin shutdown signaling, aiming to reduce allocations, improve correctness during shutdown, and streamline handler logic.
Changes:
- Replace
atomic.Pointer[bool]shutdown flag usage withatomic.Boolacross plugin and handlers. - Align
/healthshutdown behavior with configuredUnavailableStatusCode, and simplify handler loops / reduce redundant map lookups. - Improve handler efficiency and behavior (pre-size report slices, range-over-values, and propagate request cancellation to jobs checks via
r.Context()).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Switch shutdown flag storage to atomic.Bool and update handler wiring; adjust shutdown semantics. |
health.go |
Use UnavailableStatusCode on shutdown; pre-size report slice; simplify named-plugin loop and avoid redundant lookups. |
ready.go |
Pre-size report slice; simplify named-plugin loop and avoid redundant lookups; shutdown flag now atomic.Bool. |
jobs.go |
Use request context for JobsState; simplify report building loop; shutdown flag now atomic.Bool. |
handler_test.go |
Update shutdown helper for atomic.Bool and adjust shutdown expectation for /health. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Shutdown path in Ready and Jobs handlers was hard-coding 503 instead of respecting the handler's configured unavailableStatusCode field, making the setting ineffective on graceful shutdown. Update the Stop() comment to match.
/health is a liveness probe and must stay 200 while the service drains so the orchestrator does not kill the draining process; /ready and /jobs return the configured unavailable code. A prior cleanup collapsed all three shutdown branches onto unavailableStatusCode, flipping /health to 503 and breaking TestShutdown503.
- connectCodeFor: CodeInternal fallback and wrapped-sentinel unwrap - Plugin.Init: Disabled when config section missing and on unmarshal error
The coverage job ran go test with -coverpkg=./... from tests/, which instruments only the tests module (no statements), so codecov received an empty report. - run the root unit tests with coverage - point the e2e -coverpkg at the production module - upload both profiles for codecov to merge - add contents: read permissions and persist-credentials: false
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #128 +/- ##
===========================================
+ Coverage 0 92.43% +92.43%
===========================================
Files 0 6 +6
Lines 0 370 +370
===========================================
+ Hits 0 342 +342
- Misses 0 16 +16
- Partials 0 12 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Align the package doc, schema, and the Stop() comment with the liveness behavior: /ready and /jobs return the configured unavailable code on shutdown while /health stays 200. Also note /jobs honors unavailable_status_code.
The e2e step wrote ../coverage-ci/e2e.out but relied on the unit step's mkdir running first; create the dir in the e2e step too so it is self-contained.
Applied fixes
R/High — health.go:27
/healthreturned200during shutdown instead ofunavailableStatusCode. Fixed to match/readybehaviour — both now returnunavailableStatusCode(503 by default) when shutdown is in progress.M/Med — plugin.go:68
atomic.Pointer[bool]→atomic.Bool. Removesnew(bool)allocations and pointer dereferences inInit,Stop, and all three handler constructors (NewHealthHandler,NewReadyHandler,NewJobsHandler). Handler structs updated accordingly; test helpernewShutdownPtrupdated to match.R/Med — health.go:111 / ready.go:114
Named-plugin path looked up
svcto checkok, then discarded it and re-looked up the same key to call.Status()/.Ready(). Now uses the already-boundsvcdirectly.M/Low — health.go:111 / ready.go:114 / jobs.go:47
for i := range plg { … plg[i] … }→for _, name := range plg(Go 1.22 range-over-value).R/Med — jobs.go:38
JobsState(context.Background())→JobsState(r.Context())so cancellation propagates from the HTTP request.Minor
Report slices pre-sized to
len(registry)instead of a fixedcap 2.Deps
go get -u all && go mod tidyon root andtests/;go work syncon the workspace. No version changes were needed (all deps already current).Summary by CodeRabbit
Bug Fixes
Tests
Chores